Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Feat: Added Timing for each move, Spectator Feature, Chat Feature , Review Page and updated the ui for it #198

Open
wants to merge 37 commits into
base: main
Choose a base branch
from
Open

Feat: Added Timing for each move, Spectator Feature, Chat Feature , Review Page and updated the ui for it #198

wants to merge 37 commits into from

Conversation

Airbornharsh
Copy link
Contributor

@Airbornharsh Airbornharsh commented Apr 21, 2024 •
edited
Loading

Closes #148 #142 #225 #226 #227 #316

  1. Each move Timing in the Table
  2. Updated the logic for User timer as it was delayed in prefix PR(Now Fixed)
  3. SPECTATOR feature
  4. Chat Feature
  5. Review Feature - you can check the moves

Updated with all the latest Changes and solved all merge Conflicts

2024-05-05-17-37-5_GVH3UUmi.mp4

@hkirat Is this good?

@Airbornharsh Airbornharsh changed the title Fix: Timer was not showing. so added it again in the UI Fix: Added Move Timings and Updated the table ui regarding for the moves Apr 22, 2024
@Airbornharsh Airbornharsh mentioned this pull request Apr 22, 2024
Open
@Airbornharsh Airbornharsh changed the title Fix: Added Move Timings and Updated the table ui regarding for the moves Feat: Added Move Timings and Updated the table ui regarding for the moves Apr 22, 2024
@Airbornharsh Airbornharsh changed the title Feat: Added Move Timings and Updated the table ui regarding for the moves Feat: Added timing for each move and updated the ui for it Apr 22, 2024
@Airbornharsh Airbornharsh mentioned this pull request Apr 22, 2024
Open
@hkirat
Copy link
Contributor

hkirat commented Apr 22, 2024

changed some of the logic in your last PR
Mind getting rid of the conflicts?

@Airbornharsh
Copy link
Contributor Author

changed some of the logic in your last PR Mind getting rid of the conflicts?

Okay I will update it with the new one

@Airbornharsh
Copy link
Contributor Author

Airbornharsh commented Apr 22, 2024 •
edited
Loading

@hkirat Updated the code

@Airbornharsh Airbornharsh changed the title Feat: Added timing for each move and updated the ui for it Feat: Added timing for each move, Spectator Feature and updated the ui for it Apr 22, 2024
@Airbornharsh
Copy link
Contributor Author

@hkirat Added Spectator Feature

2024-04-22_13-15-141.mp4

@Airbornharsh Airbornharsh changed the title Feat: Added timing for each move, Spectator Feature and updated the ui for it Feat: Added |Timing for each move, Spectator Feature` and updated the ui for it Apr 22, 2024
@Airbornharsh Airbornharsh changed the title Feat: Added |Timing for each move, Spectator Feature` and updated the ui for it Feat: Added Timing for each move, Spectator Feature and updated the ui for it Apr 22, 2024
@Airbornharsh Airbornharsh mentioned this pull request Apr 22, 2024
Open
@Airbornharsh Airbornharsh changed the title Feat: Added Timing for each move, Spectator Feature and updated the ui for it Feat: Added Timing for each move, Spectator Feature, Chat Feature and updated the ui for it Apr 22, 2024
@Airbornharsh
Copy link
Contributor Author

@hkirat

added Chat Feature

Screenshot from 2024-04-22 21-09-12

@Airbornharsh
Copy link
Contributor Author

doesnt have the latest lock file @Airbornharsh

Updated

@nimit9
Copy link
Contributor

nimit9 commented May 5, 2024

doesnt have the latest lock file @Airbornharsh

Updated

dont know why it's showing a whitespace diff in yarn.lock

@Airbornharsh
Copy link
Contributor Author

doesnt have the latest lock file @Airbornharsh

Updated

dont know why it's showing a whitespace diff in yarn.lock

I tried to remove it but showing whitespace

@Airbornharsh
Copy link
Contributor Author

@hkirat check it out
Solved lot of conflicts which took more time than creating these features

@nimit9 nimit9 mentioned this pull request May 10, 2024
Open
@Airbornharsh Airbornharsh mentioned this pull request May 10, 2024
Open
hkirat
hkirat reviewed May 12, 2024
View reviewed changes
apps/frontend/src/App.tsx
path="/login"
element={<Login />}
path="/spectate/:gameId"
element={<Layout children={<Spectate />} />}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should ideally happen on the same url
We shouldnt expect spectators to come at a different url
fine for now tho

hkirat
hkirat reviewed May 12, 2024
View reviewed changes
apps/frontend/src/components/ChessBoard.tsx Outdated
@@ -305,30 +322,53 @@ export const ChessBoard = memo(({
to: squareRepresentation,
});
}
const timeTaken =
time - new Date(myMoveStartTime).getTime();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this be the same on both the side?
Should we wait for some ack from the server before setting this?

hkirat
hkirat reviewed May 12, 2024
View reviewed changes
apps/frontend/src/components/ChessBoard.tsx Outdated
before: moveResult?.before,
after: moveResult?.after,
piece,
createdAt: myMoveStartTime,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not a good idea, people can fake the time they did a move on

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did in backend

hkirat
hkirat reviewed May 12, 2024
View reviewed changes
apps/ws/src/Game.ts
san: move.san
before: move.before ?? '',
after: move.after ?? '',
createdAt: move.createdAt,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldnt be client side
User will send a very old time to decrease their time and cheat
Needs to be set on the server

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

hkirat
hkirat requested changes May 12, 2024
View reviewed changes
Copy link
Contributor

@hkirat hkirat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One big bug that should be fixed
Time should not be client side but calculated on the server

PLease tag me once fixed

@Airbornharsh
Copy link
Contributor Author

One big bug that should be fixed Time should not be client side but calculated on the server

PLease tag me once fixed

Check it out

@Airbornharsh Airbornharsh requested a review from hkirat May 12, 2024 20:10
hkirat
hkirat reviewed May 12, 2024
View reviewed changes
apps/frontend/src/components/ui/card.tsx
))
CardTitle.displayName = "CardTitle"
));
CardTitle.displayName = 'CardTitle';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a lot of noise?
Mind reverting these?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was not formatted before
so formatted

@Airbornharsh Airbornharsh requested a review from hkirat May 13, 2024 11:34
@Airbornharsh
Copy link
Contributor Author

@hkirat Can check it out

@Jashmit918 Jashmit918 mentioned this pull request Jul 10, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hkirat hkirat Awaiting requested review from hkirat

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Timer logic
3 participants
@Airbornharsh @hkirat @nimit9